Conversation
194f781 to
0b16113
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #73326 [ run ] triggered by Bot. Commit: |
|
PR_Github #73326 [ run ] completed with state
|
mikeiovine
left a comment
There was a problem hiding this comment.
Stamp on behalf of runtime devs, delegating proper review to @NVIDIA/trt-llm-torch-attention-devs; please ping me if you think this is not accurate
juney-nvidia
left a comment
There was a problem hiding this comment.
Approved from API perspective.
| num_kv_heads=int(num_kv_heads), | ||
| head_dim=int(head_dim), | ||
| num_index_heads=int(sparse_params.num_index_heads), | ||
| num_index_heads=( |
There was a problem hiding this comment.
For the unfused path, all 4 index heads remain replicated, whereas the fused path shards them alongside the KV heads. Does this intentionally change the indexer semantics under TP > 1?
With 4 global index heads and 4 global KV heads:
- At TP4, the unfused path takes the maximum over all 4 index heads for the single local KV head.
- At TP2, the unfused path groups the 4 index heads into pairs and takes the maximum within each pair for the 2 local KV heads.
- The fused path / TP1 case maintains a one-to-one mapping between local index heads and KV heads.
There was a problem hiding this comment.
Thanks for flagging the mismatch! The original fused sharding was correct, so I’ve fixed the unfused path to use the same KV-aligned index-head mapping and added regression coverage.
c8ee4d3 to
d40ad2e
Compare
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
PR_Github #73934 [ run ] triggered by Bot. Commit: |
pengbowang-nv
left a comment
There was a problem hiding this comment.
LGTM from attention part.
|
|
||
| return MiniMaxM3SparseParams( | ||
| num_index_heads=self.sparse_num_index_heads, | ||
| global_num_kv_heads=( |
There was a problem hiding this comment.
NIT, non-blocking: do we expect this to ever be None? If now we can remove the Optional design.
|
Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements. |
|
PR_Github #73934 [ run ] completed with state
|
Signed-off-by: peihengh <259410613+peihu-nv@users.noreply.github.com>
Signed-off-by: peihengh <259410613+peihu-nv@users.noreply.github.com>
Signed-off-by: peihengh <259410613+peihu-nv@users.noreply.github.com>
Signed-off-by: peihengh <259410613+peihu-nv@users.noreply.github.com>
Signed-off-by: peihengh <259410613+peihu-nv@users.noreply.github.com>
…tion Signed-off-by: peihengh <259410613+peihu-nv@users.noreply.github.com>
Signed-off-by: peihengh <259410613+peihu-nv@users.noreply.github.com>
The horizontal producer uses cached FP32 RoPE coefficients, while the separate main producer computes powf and __sincosf. Near FP8 midpoints these can round to adjacent E4M3 values. H100 reproduction finds one Q and one K mismatch in the 129-token, one-KV-head case; aligning norm accumulation does not remove either mismatch. Compare Q/K using ordered E4M3 encodings with a strict one-step bound. Keep V and graph replay checks exact, leave production kernels unchanged, and test the comparison across both signs, exponent boundaries, subnormals, signed zero, two-step errors, and NaNs. Signed-off-by: peihengh <259410613+peihu-nv@users.noreply.github.com>
d40ad2e to
6c046bb
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #74017 [ run ] triggered by Bot. Commit: |
Description
MiniMax-M3 MSA currently launches separate QKV and index projections, then
normalizes, applies RoPE, and inserts the projected K/V tensors into their
paged caches through separate producers. This adds projection and cache-write
overhead to prefill, mixed, and decode iterations and prevents the complete
projection path from integrating cleanly with piecewise CUDA graphs.
Only the projection and producer stages are fused. Index scoring, block Top-k
selection, and the sparse-attention kernel remain separate downstream stages.
“Current K/V” is produced for the token or chunk being processed; “historical
K/V” is read from the already-populated cache using the selected block IDs.
This change adds an opt-in fused MiniMax-M3 projection path:
Q/index-Q, and writes main K/V plus index-K directly to their paged HND FP8
cache storage;
and safe containment of padded or out-of-range cache slots;
insertion and sparse attention remain at the eager attention boundary; and
target/draft page geometry, multi-token verification, attention-DP dummy
lifecycle, and reduced-residual hidden-state capture.
The option defaults to disabled and is accepted only by the MiniMax-M3 MSA
implementation. Fusing the projection changes weight ownership, local head
geometry, cache insertion, and the prepopulated-KV attention contract; the
producer and its Eagle3/piecewise integration therefore need to land together
to avoid an unusable intermediate configuration.
Performance
A controlled B300 aggregate TP8/EP8 attention-DP A/B used an exact 8K-input /
1K-output workload at concurrency 256 for 2,560 requests. Both arms used the
same image, model, packed projection, and runtime configuration and completed
identical input and output token counts. The isolated change extended the
fused producer from pure prefill to mixed batches and CUDA-graph decode:
This result isolates the mixed/decode extension, not the full fused path
against an unfused baseline. Its benefit is also workload-dependent: a
separate matched disaggregated GEN-only pair measured 1.32% better reciprocal
median TPOT and effectively flat total throughput (-0.31%), because it exposes
only the decode-side portion of the eliminated work.
Feature-branch origin
This current-main port consolidates the final behavior developed on the
feat/m3_with_msaside branch in four PRs:projection and producer;
decode support;
and Eagle3 coverage; and
test semantics and test registration.
These PRs are provenance, not prerequisites. This PR reconstructs their
combined result against current
main.Dependency: #16021 will provide the consolidated MiniMax-M3 MSA/Eagle3 foundation; this PR will be rebased onto it and drop overlapping runtime changes once it lands.
Test Coverage
model/Eagle3/piecewise tests, and 12 MSA backend tests.
import, native-op, MSA, and serving-CLI validation in a fresh allocation.
one-model Eagle3, piecewise context graphs, and default sampling completed
54/54 warmup and 179 profiling requests with
submission_valid=true, norequest/model/sampler errors, and no cancellations.
git diff --check, and theregenerated LLM-args telemetry manifest passed.
CI execution.
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.Dev Engineer Review
QA Engineer Review
Added test coverage for:
The
tests/integration/test_lists/l0_b200.ymltest list adds entries for the FP8 horizontal producer, indexer, and main KV insertion tests.The
tests/integration/test_lists/qa/llm_function_core.txttest list adds two MiniMax-M3 NVFP4 Eagle3 accuracy cases with attention-DP disabled and enabled.The MSA backend, MiniMax-M3 model, FP8 producer, main KV insertion, accuracy, Eagle3, and executor test functions are not all represented by corresponding test-list entries in the provided changes.
Verdict: needs follow-up.